Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Override PIP_USER if running_under_virtualenv #6984

Closed
wants to merge 6 commits into from
Closed

Override PIP_USER if running_under_virtualenv #6984

wants to merge 6 commits into from

Conversation

jonathonf
Copy link

@jonathonf jonathonf commented Sep 5, 2019

This PR sets an environment variable to override any PIP_USER setting when being used within a virtual environment which sets a VIRTUAL_ENV environment variable.

This works around issues such as #2081, #4141 and #5702 .

It's a pretty naive solution, and I've tested only under Linux so this may be an incomplete solution for macOS/Windows.

@jonathonf jonathonf marked this pull request as ready for review September 5, 2019 22:14
@chrahunt
Copy link
Member

chrahunt commented Sep 5, 2019

Hello! From the latest comment by @pradyunsg on #5702 (comment) it looks like the intent is to make this an error.

@jonathonf
Copy link
Author

jonathonf commented Sep 5, 2019

I don't know if there is more to it, but from reading the other issues it appears it going to be made an error because --user doesn't work within a virtual environment.

As far as I can tell this change works around the incompatibility of having a --user setting in a pip.conf and using a virtual environment, so if there aren't any non-obvious side-effects this should be a better option.

@chrahunt
Copy link
Member

chrahunt commented Sep 5, 2019

There's a couple, based on reading #5702:

  1. A user providing --user on the command-line would get unexpected behavior
  2. A user providing PIP_USER=yes in their environment would get unexpected behavior

We want to provide the ability to have the desired behavior (use virtualenv if in virtualenv, use user if not), but the UI/UX around it is important otherwise we end up with issues and user confusion/frustration.

@jonathonf
Copy link
Author

Adding PIP_USER=yes within the environment would be the equivalent of setting global.user=yes in a configuration file so I don't think that would cause too much confusion, and given PIP_USER=yes would currently cause an issue within a venv anyway then effectively ignoring that variable in a context it makes no sense to be used in keeps things working.

As for --user within a venv... I'd argue that it's implied that a venv is a "user-type" environment so using --user is nonsensical and so it should be a no-op (same as other commenters in the other issues).

In any event, this change makes behaviour incrementally less unexpected than the current state.

we end up with issues and user confusion/frustration

I'd say this is already the case, and has been since at least 2014. 😉

@jonathonf jonathonf changed the title Override PIP_USER if inside a VIRTUAL_ENV Override PIP_USER if running_under_virtualenv Sep 8, 2019
Copy link
Member

@chrahunt chrahunt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would also need tests for this, but I'm not sure it's worth doing until other people weigh in and can confirm this is a good approach that we want to take while working towards that other issue.

src/pip/_internal/configuration.py Outdated Show resolved Hide resolved
@jonathonf
Copy link
Author

jonathonf commented Sep 25, 2019

I'm not sure it's worth doing until other people weigh in and can confirm this is a good approach that we want to take while working towards that other issue.

I can understand the rationale. I'm just also aware that this has been "open" since 2014 and it doesn't appear that any progress has been made, and that sometimes "something is better than nothing".

Another approach is over here: #7002

until other people weigh in

Is there any way of poking other people who could weigh in? Even if this particular PR is no good it might trigger some ideas.

@pradyunsg
Copy link
Member

I am not sure whether we want to go down this route, but the way this is implemented currently is definitely not how we'd want to implement this.

I'd rather we amend the current check for aborting:

diff --git a/src/pip/_internal/commands/install.py b/src/pip/_internal/commands/install.py
index 66071f6e..bb97dbbb 100644
--- a/src/pip/_internal/commands/install.py
+++ b/src/pip/_internal/commands/install.py
@@ -298,10 +298,11 @@ class InstallCommand(RequirementCommand):
                     "different installation locations"
                 )
             if virtualenv_no_global():
-                raise InstallationError(
-                    "Can not perform a '--user' install. User site-packages "
-                    "are not visible in this virtualenv."
+                logger.info(
+                    "Ignoring '--user', since user site-packages are not "
+                    "visible in this virtualenv."
                 )
+                options.use_user_site = False
             install_options.append('--user')
             install_options.append('--prefix=')

@pradyunsg
Copy link
Member

I've filed #7164 for discussion on this functionality.

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Oct 22, 2019
@pradyunsg
Copy link
Member

Closing since #7002 has merged.

@pradyunsg pradyunsg closed this Oct 22, 2019
@jonathonf jonathonf deleted the virtual-env-user branch October 22, 2019 13:32
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Nov 21, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation needs rebase or merge PR has conflicts with current master
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants